[ENG-11015] [ENG-11692] IDS Phase 1 & 2 Project PR - WB Part#492
Conversation
… in OSFStorageProvider (#483) * Add minimal query parameter to zip and metadata methods in OSFStorageProvider * Fix minimal query parameter handling in _children_metadata method
…rguments for metadata retrieval (#484)
Add minimal metadata fixture and update OSFStorageProvider to handle minimal parameter
* Update zipfile name format in download_folder_as_zip method to include resource identifier * Update zipfile naming convention in download_folder_as_zip method to include resource identifier
…dpoint for OSFStorage Zip (#487) * Add pagination support to ZipStreamGenerator and update OSFStorageProvider for minimal metadata retrieval * Refactor OSFStorageProvider to enhance pagination handling and limit parameter for metadata retrieval
* add cache to metadata calls for ZipStreamGenerator * implement cache as provider proxy; revert zip generator * fix circular import error * Apply suggestions from code review * Minor code style update + docstring Co-authored-by: Longze Chen <cslzchen@gmail.com> --------- Co-authored-by: Longze Chen <cslzchen@gmail.com>
* minor paginated metadata improvements * use metadata method instead of _children_metadata when iter_children_pages
* pass storage meta to provider download method * fix tests * add comments and docstrings * move sentry call and remove dead code
Add unit tests for DAZ improvements phase 2
|
coverage: 84.519% (-0.2%) from 84.708% — feature/improve-daz-phase2 into develop |
* Remove orm query param for daz metadata
| return base.child(path, folder=folder) | ||
|
|
||
| async def zip(self, path: wb_path.WaterButlerPath, **kwargs) -> asyncio.StreamReader: | ||
| async def zip(self, path: wb_path.WaterButlerPath, use_cache: bool = True, paginated: bool = False, **kwargs) -> asyncio.StreamReader: |
There was a problem hiding this comment.
For code reviewer: metadata prefetch/cache and pagination is mutually exclusive, our testing found the former performs better with consistency, this is why it's the default option.
|
|
||
| class ZipStreamGenerator: | ||
| def __init__(self, provider, parent_path, *metadata_objs): | ||
| class BaseZipStreamGenerator(abc.ABC): |
There was a problem hiding this comment.
For code reviewer: we have two generators: ZipStreamGenerator and ZipStreamGeneratorPaginated, both of which inherit from this base/abstract one.
| return url, data, params | ||
|
|
||
| async def download(self, path, version=None, revision=None, mode=None, **kwargs): | ||
| async def download(self, path, version=None, revision=None, mode=None, metadata=None, **kwargs): |
There was a problem hiding this comment.
For code reviewer: metadata is only provided during DAZ. See __anext__ in both "BaseZipStreamGenerator"s
| use_embedded_storage = False | ||
| if metadata is not None: |
There was a problem hiding this comment.
For code reviewer: use_embedded_storage is used to decide whether to skip the extra request during file download. And it can only be True (though not necessary) if metadata is provided (as mentioned before, this means during DAZ only).
| use_embedded_storage = version is None and bool(storage and storage.get('data', None)) | ||
| if not use_embedded_storage: | ||
| with sentry_sdk.new_scope() as scope: |
There was a problem hiding this comment.
For code reviewer: During DAZ, we expect version is None and bool(storage and storage.get('data', None)) to be True. However we added the sentry message to check if we ever fail to get the storage data in one request. So far, we haven't seen this error in staging1 sentry.
| else: | ||
| # osf storage metadata will return a virtual path within the provider | ||
| resp = await self.make_signed_request( | ||
| 'GET', | ||
| self.build_url(path.identifier, 'download', version=version, mode=mode), | ||
| expects=(200, ), | ||
| throws=exceptions.DownloadError, | ||
| ) | ||
| data = await resp.json() |
There was a problem hiding this comment.
For code reviewer:
- In standard download, this one is always executed.
- In DAZ, this one is a fallback when we can't get the storage data in metadata request.
| async def zip(self, path: wb_path.WaterButlerPath, **kwargs) -> asyncio.StreamReader: | ||
| """Streams a Zip archive of the given folder | ||
|
|
||
| :param path: ( :class:`.WaterButlerPath` ) The folder to compress | ||
| """ | ||
| return await super().zip(path, **kwargs, minimal=True) |
There was a problem hiding this comment.
For code reviewer: overrides zip with minimal=True for osfstorage, this allows osfstorage to use the improved BE endpoint without affecting zip used by other add-ons.
fyi, the BE code in https://github.com/CenterForOpenScience/osf.io/pull/11804/changes#diff-59778b64c2f6ccf2186d779d90f65d37988ac47b3be938c4eb672130a0561b11R355
@must_be_signed
@decorators.autoload_filenode(must_be='folder')
def osfstorage_get_children(file_node, **kwargs):
if is_truthy(request.args.get('minimal')):
return _osfstorage_minimal_metadata(
file_node,
limit=request.args.get('limit', type=int, default=None),
after=request.args.get('after', type=int, default=None),
)
return _osfstorage_full_metadata(file_node, request.args.get('user_id'))
5 participants
Ticket
Purpose
IDS Phase 1 & 2 Project PR - WB Part
Here is the BE Part: CenterForOpenScience/osf.io#11804
Changes
See individual PRs
Side Effects
N/A
QE Notes
See test planning
CE Notes
See release playbook
BE should be merge/deployed before WB.
Documentation
N/A